Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace aria-selected with aria-hidden #774

Merged
merged 2 commits into from
Jan 24, 2019
Merged

Conversation

danielkorte
Copy link
Contributor

Problem
[aria-*] attributes do not match their roles for slides. Each ARIA role supports a specific subset of aria-* attributes. Mismatching these invalidates the aria-* attributes.

Solution
The aria-selected is not appropriate here. Both examples below use aria-hidden which is an available ARIA attribute for almost any element.

@desandro
Copy link
Member

desandro commented Jun 3, 2018

Hello! Thanks for this contribution. I could certainly use help with ARIA.

I currently use aria-selected over aria-hidden because Flickity carousels allows for multiple cells to be viewable in the viewport AND multiple cells to be selected with groupCells. If there's a better ARIA attribute to use to accommodate that behavior, I'm open!

@danielkorte
Copy link
Contributor Author

danielkorte commented Jun 4, 2018

Yes, aria-hidden is a better ARIA attribute to use than aria-selected.

The patch is more or less a straight replacement of aria-selected for aria-hidden so it works the same on multiple cells and groupCells as the current implementation. The primary issue is aria-selected is inappropriate for a div (any ARIA role) or list item (listitem ARIA role) because neither role allows aria-selected as an attribute. The aria-selected attribute is reserved for gridcell, option, row, tab, columnheader, menuitemradio, radio, rowheader, and treeitem ARIA roles:
https://www.w3.org/WAI/PF/aria/states_and_properties#aria-selected

@desandro
Copy link
Member

desandro commented Jun 5, 2018

Thanks for providing this info. I still feel that aria-hidden is not the correct attribute, as it signifies that the content is not visible. That said, perhaps a better solution is to select a role to use — although there doesn't seem to be an obvious option.

@danielkorte
Copy link
Contributor Author

I believe the thinking is that slides not visible to sighted users (off screen or out of view of the flickity viewport in this case) should also not be available to unsighted users (via aria-hidden). The two links in the first comment both implement their carousels with aria-hidden so I'd trust what the W3 and Athena ICT is recommending...

@desandro
Copy link
Member

desandro commented Jun 8, 2018

Okay then. If you want to add aria-hidden to cells outside the viewport, that would require additional code to calculate which cells are and are not visible. That I can get behind. But for now, this PR only changes aria-hidden on the selected cells.

@danielkorte
Copy link
Contributor Author

I don't see a need for additional code. The patch is a replacement of the aria-selected attribute for aria-hidden attribute. The inactive slides are the ones that need to be targeted with aria-hidden which is what is already happening with aria-selected="false" in the current implementation.

@desandro
Copy link
Member

Okay. I feel we're talking past each other now. I appreciate your contribution and the discussion, but I'm going to pass on this PR.

@desandro desandro closed this Jun 10, 2018
@danielkorte
Copy link
Contributor Author

danielkorte commented Jun 10, 2018

That's unfortunate. If you'd like test results you can simply use Chrome to observe the difference. Please let me know if you have any further questions or decide to reconsider.

Chrome > Audits > Perform an audit...

Results for https://flickity.metafizzy.co/
screen shot 2018-06-10 at 11 05 11 am

Results with my patch:
screen shot 2018-06-10 at 11 11 51 am

@kobeaerts
Copy link

I agree with Daniel on this issue and would like to see the aria-selected replaced with aria-hidden

@desandro
Copy link
Member

First step would be adding API to get visible elements in viewport. #638

@danielkorte
Copy link
Contributor Author

I respectfully disagree. There is no reason this patch can‘t (or shouldn‘t) be implemented right now without an “API to get visible elements in the viewport.” This patch is nothing more than a fix to a misused and misunderstood ARIA attribute (aria-selected). It is true that such an API would improve this accessibility feature since using wrapAround breaks it with or without the patch, but an API is not a required first step to fixing which ARIA attribute is actually used.

@desandro desandro reopened this Jun 29, 2018
@jonnitto
Copy link

When this will get merged?

@desandro desandro self-assigned this Jan 23, 2019
@desandro desandro merged commit 4475acf into metafizzy:master Jan 24, 2019
@desandro
Copy link
Member

This feature is now merged and will go out in Flickity v2.2.0.

@danielkorte Thank you for your contribution and for helping me see the light on this one. Sorry it took so long to make it in

desandro added a commit that referenced this pull request Jan 29, 2019
@desandro
Copy link
Member

This fix has been released with v2.2.0. Thanks again!

@vasanthangel4
Copy link

Hi @desandro : when we enabling the voice over and accessibility carousel scrolling not working with Three-finger triple tap in Safari browser in all ios device. android application is working fine. please let us know if you have solution. i have applied accessibility improvement PR, but it is not helping. please let me know if any vanila js fix for this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants